Skip to content

Fix wrong rows, unfiltered selects and ignored sorts in relationship queries - #3775

Merged
kriszyp merged 10 commits into
mainfrom
fix/relationship-filter-select-correctness
Oct 30, 2026
Merged

kriszyp merged 10 commits into
mainfrom
fix/relationship-filter-select-correctness

Conversation

@kriszyp

@kriszyp kriszyp commented Oct 29, 2026 •

Copy link
Copy Markdown
Member
Kyle checks each child box label against the order slip, sets a box with a missing label aside and passes on only the boxes that match every line

⊙ Problem

Relationship queries gave wrong results:

  • ?id=p1&children.id=c1 dropped the matching parent, and ?id=p1&children.id=p1 matched it by its own id.
  • An array foreign key holding a dangling id returned a 500.
  • A second condition, a multi-hop condition or a keyless related table left select() unfiltered.
  • Sorting by a to-many attribute was ignored.
  • Nested selections with conditions blocked on reads.

🤔 Is fixing all of these in one PR right?

Yes, as briefed. They share filterByType and the select-restriction map it feeds. One pair of tests covers them. The async-read fix for nested selections is its own commit.

💡 Solution

⚖️ Alternatives

  • Run relationship key conditions first — enforceExecutionOrder and nested-selection predicates bypass the planner.
  • One restriction tree per AND/OR scope — the real fix for OR groups, which keep first-branch-wins for now: harper#3762.

🤔 Register AND restrictions when the filters are built?

Chosen over lazy registration on first use. Lazy registration made the restriction depend on which row ran first, and nested selections read it before any row ran. Side effect: an id-shortcut condition now restricts select() too, as a leading join already did.

🔧 Changes

🤔 Sort by a to-many attribute: element by element?

Parents compare by the array of related values in relationship order, the same way an array column sorts. Min or max per direction is the alternative. Once released, changing this changes visible order.

🤔 One extra call per projected row

The final projection step moved into selectRecord so a pending recheck can finish first. That adds one call per row alongside the closure that step already allocates per row. I could not measure it because another session held the machine's benchmark lock.

🤔 Nested selections after a cancel or a timeout

A predicate already pending when its selection is cancelled can issue one more bounded read. After a transaction timeout, the row in progress can still finish its nested selection. Those last steps do no reads, the timeout leaves the snapshot open so iterators can finish, and the commit then fails with the timeout error.

Accepted minor: relationships named filters, fromRecord or hasMappings get no select restriction.

✅ Verification

  • Two acceptance tests, adapted from the staged QA candidates: relationshipPrimaryKeyFilters.test.js (REST + search) and relationshipQueryContracts.test.js (read-count spies). Against origin/main sources with a forced rebuild, 14 of 20 failed on rocksdb and on lmdb. Now 23/23 pass on both. The 3 cases added during review each fail on the commit before their fix.
  • Existing relationship suites: rocksdb 462, lmdb 446 passing. Relationship integration tests: 40/40.
  • Live server over REST (/verify): ?id=p1&children.id=c1 → ["p1"], children.id=p1 → [], references.state=closed → 200, sibling conditions narrow children, to-many sort orders both directions.
Full gates on a loaded host
  • test:unit:resources (rocksdb): 5780 passing, 2 failing. Both failures pass alone (subscriptionReplay, updateAttributesLock watchdog).
  • LMDB resources: 4336 passing with subscriptionReplay excluded. That file segfaulted in the full run and passes alone twice.
  • test:unit:main: cut off at 58 min with turbo off. Its deployActivation failures pass alone in 0.7 s.
  • The full integration suite is left to CI.

PR QA: Do relationship filters, select restrictions and to-many sorts match a brute-force oracle at scale, across pages and under concurrent child writes? — queued @ 814e555, qa-pr-harper-3775-814e555 (later heads add only a read-guard check and a DESIGN.md fold)

Docs: queued docs-harper-3775 (sorting by an attribute across a to-many relationship); the select-restriction behavior is already documented.

🤖 Generated by Claude Opus 5.5 (Claude Code); posted via @kriszyp.

Release target: main — relationship query fixes, partly in code #3279 and #3615 added on main only; no backport requested
Related PRs: #3619 overlaps (merged: same relationship read paths; the merge with main passes the relationship suites), #3724 overlaps (same mapConditionsToFilters signature), #3316 overlaps (relationship read permissions in the same selection paths), #3771 overlaps (search cancellation beside selectionStopped), 11 others independent
Complexity: complicated

Deferred findings

Late Grok findings

9 surviving (highest major) — Grok reviewed 30a3042
  • major — resources/Table.ts:8532 — A cached child passes the first predicate, then its recheck goes async (pendingMatch) while a related-select projection (projected) is already running. Nothing observes projected until pendingMatch settles. If the projection rejects first (for example ReadSnapshotExpiredError), Node raises
  • major — resources/Table.ts:11649 — Each to-many hop opens its own window of 16 reads (RELATED_READS_IN_FLIGHT), so two to-many hops in one sort path (children.siblings.name) put 16×16 = 256 reads in flight. resources/DESIGN.md:269 says the cap still holds after a to-many hop. The wide-sort test has only one to-many hop, so it c
  • minor — resources/search.ts:1807 — The selection object has a filters key and is also passed down as the next level's relationship map. For a to-many relationship named filters, a non-leading condition like children.filters.flag reads that metadata array as filterMap and adopts it as the selection. selection.filters.push at
  • minor — resources/Table.ts:7653 — Cancellation is not rechecked after an awaited read:
  • minor — unitTests/resources/relationshipQueryContracts.test.js:1 — The test uses require('node:assert/strict'), which house style prohibits. Switch to plain node:assert with explicit strictEqual/deepStrictEqual
  • minor — resources/search.ts:1890 — In the sync relationship filter, subObject.some(nextFilter) passes the array index as entry, and the subEntries it builds are thrown away (deferred to a follow-up)
  • minor — resources/search.ts:1806 — No select restriction is applied for to-one→to-many hops, OR multi-hop conditions, or OR branches that take the id shortcut (deferred to a follow-up)
  • nit — resources/DESIGN.md:270 — The note documents the OR-scope "first evaluated condition wins" gap but links no tracking issue
  • nit — resources/Table.ts:11648 — New comments that narrate the code, aggregated into one nit:

Not standing: none. Grok ran 27m (27m from launch to report); the fold adjudication (cursor-grok@30a30424172f-c9897035, claude) took 1m.

Late Grok findings

1 surviving (highest minor) — Grok reviewed 3623247, ruled against 0e7b7ae
  • minor — resources/Table.ts:7545 — When a relationship filter sits inside an OR and the select also passes conditions, sort, limit or offset, the select is not narrowed by that filter. _prepareRelationshipSelection reads the filter's restriction once, at resources/Table.ts:7545, while the select step is being built (`reso

Not standing: none. Grok ran 23m (24m from launch to report); the fold adjudication (cursor-grok@36232476e2f1-f71b1bb8, claude) took 1m.

Late Grok findings

5 surviving (highest minor) — Grok reviewed 12d1f4a, ruled against 814e555
  • minor — resources/search.ts:1870 — a to-one hop (no elements) and any OR-scoped multi-hop register no select restriction. preserveForSelect at resources/search.ts:1904-1907 stores only the outer predicate, so select still returns related rows that the condition rejected. This is already deferred as `select-restriction-missing
  • minor — resources/search.ts:1870 — a to-many relationship named filters, fromRecord or hasMappings is excluded by SELECTION_BOOKKEEPING (resources/search.ts:1536), so restrictsSelect is false. Example: with related rows g1 (code=yes) and g2 (code=no), filters.code = yes projects both rows
  • minor — resources/Table.ts:7659 — cancellation is applied only after matchCandidate settles. If a cancel lands while a nested predicate (for example a loadEntries on grand.great.code) is pending, its storage reads still run to completion and hold a background read slot until they settle
  • minor — resources/Table.ts:7778 — four continuations resume without calling checkActive:
  • nit — unitTests/resources/relationshipQueryContracts.test.js:360 — the plain OR select test (:360) and the OR test with a limit (:380) assert only rows[1]. rows[0] (or-a, whose child c7 matched) is unasserted because it currently returns children: [] under the deferred first-wins behaviour. A regression on or-a stays green

Not standing: 1 separate-issue, 2 merged. Grok ran 16m (16m from launch to report); the fold adjudication (cursor-grok@12d1f4a0ccf8-3d79cb00, claude) took 1m.

Late Grok findings

5 surviving (highest minor) — Grok reviewed 39fcf53
  • minor — resources/Table.ts:7660 — when a nested candidate predicate returns false after the transaction has timed out, the code returns SKIP before checkActive runs. If that is the last candidate, the parent row is produced without transactionOpenTooLongError. The join scan at resources/search.ts:1512-1513 checks the guard w
  • minor — resources/Table.ts:7782 — the offset and projection continuations check only stopped, which stays false on abortDueToTimeout. They then push or finish() with no read guard, so the last row lands without the timeout error
  • nit — resources/Table.ts:7659 — cancellation is seen only after matchCandidate settles, so a nested predicate's in-flight related reads keep running after close(). This is bounded to one predicate evaluation, and the resume returns SKIP
  • nit — resources/search.ts:1870 — a to-many relationship named filters, fromRecord or hasMappings gets no select restriction, so non-matching related rows are projected. This is documented in resources/DESIGN.md:394. The test at unitTests/resources/relationshipQueryContracts.test.js:451-461 has only one related row, so it
  • nit — unitTests/resources/relationshipQueryContracts.test.js:360 — both OR select tests (lines 360 and 380) assert only rows[1]. The wrong or-a.children result ([], although child c7 matched) is invisible to them. That gap is the deferred first-wins restriction defect, tracked in harper#3762

Not standing: 1 separate-issue. Grok ran 11m (11m from launch to report); the fold adjudication (cursor-grok@39fcf537c09b-c7ed3f1d, claude) took 1m.

Origin — the dispatch brief this PR was written from

Fix relationship-path filter/select/sort defects in resources/search.ts (filterByType ~1784-1895, preserveForSelect ~1858-1882, attributeComparator) and resources/Table.ts (_prepareRelationshipSelection ~7529-7675, getPathValue ~11556-11592). From F-1046: (1) the recordFilter idFilter shortcut (~1828-1845, and its maybeAsync copy ~1891) builds subIdFilter from resolver.from ?? primaryKey even when resolver.to is set, comparing the related PK against the wrong field: guard with (!resolver.to || idFilter.idSet); (2) allowObjectMatching=true makes an array FK (from: childIds) match as a whole array: use !resolver.toMany || Array.isArray(relatedTable.primaryKey); (3) getSubObject's subEntry.map(e => e.value) throws on a dangling FK (HTTP 500): skip nullish entries. From F-1051: (4) preserveForSelect binds fromRecord only for the first non-leading condition (!filtered[firstAttributeName]); accumulate siblings; (5) fromRecord uses value[relatedTable.primaryKey], undefined for composite keys: use entry.key ?? value[pk]; (6) the recursive call passes filterMap?.[name]?.joined (~1794) instead of the sibling map; (7) getPathValue does value?.[attr] on an Array for to-many relationships on the path: map over elements; (8) readCandidate calls getEntry without async: true / freezeRelatedEntry and compile drops maybeAsync (predicate() called synchronously), violating the invariant #3615 wrote into resources/DESIGN.md. If (8) grows large, land it as a second commit in the same PR.

Acceptance

Both staged tests pass on rocksdb and lmdb (P-6647 asserts async-read counts for item 8); existing relationship suites stay green. Show each new test failing on origin/main sources (forced rebuild) before the fix.

Dispatch: task harper-relationship-filter-select-correctness · queued by finding-triage/wave-43 · ran by claude/opus/xhigh · worker kzyp-xps-1

Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex,cursor-grok; late=cursor-grok:adjudicated; adjudicated=domain; declined=muse; rounds=8; full=2 @ 516a50e

Review-Attention: deep ~50m (critical: Table.ts, search.ts; decisions: cancellation-granularity, reserved-relationship-names, or-scope-first-wins, do-less-alternative) @ 516a50e

kriszyp and others added 8 commits October 29, 2026 09:25
… queries

- A related primary-key condition on a `to` relationship no longer takes the
  id-filter shortcut, which compared the related key with the local key; an id
  filter mapped through `to` is tagged `mappedTo` and is the only kind it takes.
- An array foreign key is tested per element in the shortcut.
- A dangling foreign key in a to-many relationship is skipped instead of
  throwing.
- Every AND-scope condition on a to-many relationship restricts its select(),
  registered when the filter is built; OR scopes keep their lazy path. Matched
  related records are selected by entry key, so a table whose key is not a
  record field works.
- A multi-hop condition passes the related-level map down, so a second leading
  two-hop condition filters the innermost join and a non-leading one restricts
  the nested select().
- Sorting by an attribute across a to-many relationship resolves it per related
  record, bounded like loadRelatedEntries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
…usly

Under asynchronous iteration, a nested selection with conditions now reads a
key candidate (such as one from a leading join) with an async read and freezes
it, evaluates its predicate through the async-capable form, and awaits the
predicate when transformEntryForSelect rechecks it after projection. AND/OR
groups of nested predicates carry a maybeAsync form, as top-level groups do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
…ants

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
…nding recheck, safe selection lookups

- A sort through more than one to-many hop shares one RELATED_READS_IN_FLIGHT
  budget: related records resolve one at a time while a deeper to-many hop
  remains, instead of each hop opening its own window.
- transformEntryForSelect projects a row only after a pending asynchronous
  recheck settles true (the projection tail is now selectRecord).
- filterByType reads a query's filtered map by own property and recognizes a
  join Map or RelationshipSelection by type, so a relationship named like an
  inherited member or the maps' own bookkeeping keys no longer throws; a
  selection is installed only once its condition compiled.
- A nested selection candidate rechecks cancellation once its read settles.
- The synchronous relationship filter passes each related entry, not the
  array index, to the next level; an OR branch answered by the id shortcut
  registers its select restriction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
…hape

- A pending nested recheck that settles after the selection is returned no
  longer projects the row; a nested candidate checks the read-transaction
  guard once its read settles.
- A sort through a to-many relationship stops starting related reads once its
  iterator is closed (the post-ordering iterator's stopped() is passed down).
- transformEntryForSelect and _prepareRelationshipSelection read a query's
  filtered map through relationshipRestriction (own property, a join Map or an
  object with fromRecord), so a relationship named like a map bookkeeping key
  projects normally.
- resources/DESIGN.md links the OR-scope select-restriction gap to harper#3762.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
_prepareRelationshipSelection read the query's restriction for the
relationship once, when the selection was prepared. An OR-scope condition
registers its restriction while the first row is filtered, after that, so a
nested selection with a limit, sort or conditions ignored it while a plain
select honored it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
@kriszyp kriszyp added this to the v5.4 milestone Oct 29, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances relationship query handling by supporting asynchronous reads, select restrictions, and sorting across to-many relationships, backed by extensive new unit tests. The review feedback highlights two important improvements: first, attaching a dummy .catch() handler to promises returned by getPathValue in relatedPathValue and remainingRelatedPathValues to prevent unhandled promise rejections; second, calling checkActive?.() after matchCandidate resolves in _prepareRelationshipSelection to guarantee transaction safety after yielding to the event loop.

Comment thread resources/Table.ts Resolved
Comment thread resources/Table.ts Resolved
kriszyp and others added 2 commits October 29, 2026 13:42
…whose predicate awaited

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Dispatch-Task: harper-relationship-filter-select-correctness
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant